Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Ord instances for Interval, IntervalSet and IntervalMap #41

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

googleson78
Copy link
Contributor

The primary intended use case for these is to allow storage in maps and sets.

@Bodigrim
Copy link
Collaborator

I'm generally in favor of this.

@msakai what do you think?

@googleson78
Copy link
Contributor Author

ping @msakai, should I put in the effort to get CI cleaned up for all versions on this, or is this patch undesirable overall?

@msakai
Copy link
Owner

msakai commented Aug 27, 2023

Sorry for the delay. I will take a look.

@msakai
Copy link
Owner

msakai commented Aug 27, 2023

It is a bit unnatural, just like defining Ord instances of Complex, but if there is a demand or a use case that a user wants to use Map/Set instead of HashMap/HashSet, I think it is okay to add.

@msakai
Copy link
Owner

msakai commented Aug 27, 2023

@googleson78
The following change fixes CI failure at least on GHC-8.10.7. Could you try this?

diff --git a/src/Data/Interval/Internal.hs b/src/Data/Interval/Internal.hs
index 16890a4..ae26189 100644
--- a/src/Data/Interval/Internal.hs
+++ b/src/Data/Interval/Internal.hs
@@ -56,12 +56,12 @@ data Interval r
   | RightOpen !r !r
   | BothOpen !r !r
   deriving
-    ( Eq,
-      Ord,
+    ( Eq
+    , Ord
       -- ^ Note that this Ord is derived and not semantically meaningful.
       -- The primary intended use case is to allow using 'Interval'
       -- in maps and sets that require ordering.
-      Typeable
+    , Typeable
     )
 
 peekInterval :: (Applicative m, Monad m, Ord r) => m Int8 -> m r -> m r -> m (Interval r)
diff --git a/src/Data/IntervalMap/Base.hs b/src/Data/IntervalMap/Base.hs
index 556e763..f8a4979 100644
--- a/src/Data/IntervalMap/Base.hs
+++ b/src/Data/IntervalMap/Base.hs
@@ -124,12 +124,12 @@ import qualified GHC.Exts as GHCExts
 -- even if adjacent intervals are connected and mapped to the same value.
 newtype IntervalMap r a = IntervalMap (Map (LB r) (Interval r, a))
   deriving
-    ( Eq,
-      Ord,
+    ( Eq
+    , Ord
       -- ^ Note that this Ord is derived and not semantically meaningful.
       -- The primary intended use case is to allow using 'IntervalSet'
       -- in maps and sets that require ordering.
-      Typeable
+    , Typeable
     )
 
 #if __GLASGOW_HASKELL__ >= 708
diff --git a/src/Data/IntervalSet.hs b/src/Data/IntervalSet.hs
index 7e44647..fac557e 100644
--- a/src/Data/IntervalSet.hs
+++ b/src/Data/IntervalSet.hs
@@ -89,12 +89,12 @@ import qualified GHC.Exts as GHCExts
 -- Any connected intervals are merged together, and empty intervals are ignored.
 newtype IntervalSet r = IntervalSet (Map (Extended r) (Interval r))
   deriving
-    ( Eq,
-      Ord,
+    ( Eq
+    , Ord
       -- ^ Note that this Ord is derived and not semantically meaningful.
       -- The primary intended use case is to allow using 'IntervalSet'
       -- in maps and sets that require ordering.
-      Typeable
+    , Typeable
     )
 
 #if __GLASGOW_HASKELL__ >= 708

It seems the feature for writing comments in this location was added in GHC 8.2.1.

I think it's okay to drop support for GHC <8.2. (We can update the build matrix in a separate PR)

@Bodigrim
Copy link
Collaborator

Bodigrim commented Sep 9, 2023

Dropped support of GHC < 8.2 in 4115223. @googleson78 could you please rebase and try to fix haddocks as suggested?

googleson78 and others added 2 commits September 11, 2023 13:09
The primary intended use case for these is to allow storage in maps and
sets.
@googleson78
Copy link
Contributor Author

Added Masahiro's patch and rebased on master, 🤞

@Bodigrim Bodigrim merged commit e9150ae into msakai:master Sep 11, 2023
@Bodigrim
Copy link
Collaborator

Thanks!

@googleson78
Copy link
Contributor Author

Thanks to the both of you too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants